feat(sdk-metrics): implement metric reader metrics#6449
feat(sdk-metrics): implement metric reader metrics#6449anuraaga wants to merge 17 commits intoopen-telemetry:mainfrom
Conversation
4c1dd02 to
0e63817
Compare
0e63817 to
5beee50
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6449 +/- ##
==========================================
+ Coverage 95.76% 95.79% +0.02%
==========================================
Files 375 378 +3
Lines 12739 12780 +41
Branches 3013 3020 +7
==========================================
+ Hits 12200 12242 +42
+ Misses 539 538 -1
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| const collectDuration = hrTimeDuration(startTime, endTime); | ||
| const collectDurationSecs = collectDuration[0] + collectDuration[1] / 1e9; |
There was a problem hiding this comment.
Let me know if I missed something, I was surprised to not find hrTimeToSeconds since there are many metrics defined as seconds
There was a problem hiding this comment.
I think it would be worthwhile adding hrTimeToSeconds to "packages/opentelemetry-core/src/common/time.ts". That could totally be in a separate PR, though. Or an issue would be welcome.
| * - Use a domain-specific attribute | ||
| * - Set `error.type` to capture all errors, regardless of whether they are defined within the domain-specific set or not. | ||
| */ | ||
| export const ATTR_ERROR_TYPE = 'error.type' as const; |
There was a problem hiding this comment.
This one is actually stable but wasn't sure it's worth adding a dep on semconv for it
There was a problem hiding this comment.
Understood. I don't have a strong opinion. If the semconv package was smaller we wouldn't hesistate to add the dep. :)
I imagine this usage will break attempts to use https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/scripts/gen-semconv-ts.js to re-generate this semconv.ts file, but that's not a real concern right now. That script is currently a best-effort helper and not mandated at all.
| * - Use a domain-specific attribute | ||
| * - Set `error.type` to capture all errors, regardless of whether they are defined within the domain-specific set or not. | ||
| */ | ||
| export const ATTR_ERROR_TYPE = 'error.type' as const; |
There was a problem hiding this comment.
Understood. I don't have a strong opinion. If the semconv package was smaller we wouldn't hesistate to add the dep. :)
I imagine this usage will break attempts to use https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/scripts/gen-semconv-ts.js to re-generate this semconv.ts file, but that's not a real concern right now. That script is currently a best-effort helper and not mandated at all.
| this._metrics = new MetricReaderMetrics( | ||
| OTEL_COMPONENT_TYPE_VALUE_PERIODIC_METRIC_READER, | ||
| meter | ||
| ); |
There was a problem hiding this comment.
What about moving the MetricReaderMetrics instance to the MetricReader base class:
setMeterProvidermoves toclass MetricReader(and the method gets added tointerface IMetricReader)- The
setMeterProviderneeds a value forotel.component.type, so a class extendingMetricReadershould be able to pass that in:- Add an optional
otelComponentTypestring option tointerface MetricReaderOptions - Get
class PeriodicExportingMetricReaderto pass inOTEL_COMPONENT_TYPE_VALUE_PERIODIC_METRIC_READERin its call tosuper() - Get
class MetricReaderto save that toprivate readonly _otelComponentType: string | undefined; - Get the implementation of
setMeterProviderto usethis._otelComponentType || this.constructor.name. This will allow a reasonable default value for MetricReader subclasses that don't fit one of the well-known names forotel.component.type. (From the spec: "otel.component.type: If none of the standardized values apply, implementations SHOULD use the language-defined name of the type. E.g. for Java the fully qualified classname SHOULD be used in this case.")
- Add an optional
- Move the handling for
this._metrics.recordCollection(...)toMetricReader#collect(). - Then it would also be good to update "experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts" to pass in
otelComponentType: OTEL_COMPONENT_TYPE_VALUE_PROMETHEUS_HTTP_TEXT_METRIC_EXPORTERin its call tosuper()
There was a problem hiding this comment.
Good idea that makes sense - did it and also the prometheus change. One deviation from this comment I resolved _otelComponentType to the automatic value in the constructor instead of setMeterProvider, let me know if I'm missing some JS intricacy that means that shouldn't be done.
| } | ||
|
|
||
| const collectDuration = hrTimeDuration(startTime, endTime); | ||
| const collectDurationSecs = collectDuration[0] + collectDuration[1] / 1e9; |
There was a problem hiding this comment.
I think it would be worthwhile adding hrTimeToSeconds to "packages/opentelemetry-core/src/common/time.ts". That could totally be in a separate PR, though. Or an issue would be welcome.
trentm
left a comment
There was a problem hiding this comment.
LGTM, modulo the changelog nit.
@pichlermarc Would you like to sanity check anything here? E.g. the addition of setMeterProvider to IMetricReader, perhaps. I'm not super-confident in the Metrics SDK.
…y-js into metrics-reader-metrics
|
Thanks @trentm - updated |
I think the implementation looks solid - only issue I could see is that metrics.setGlobalMeterProvider(new MeterProvider());
const meterProvider = metrics.getMeterProvider();
const reader = new PeriodicExportingMetricReader({exporter: myExporter});
reader.setMeterProvider(meterProvider); // did not read the docs, why are no metrics exported?I wonder if |
That's a good point, thanks for bringing it up. In Java and Python, this method is actually private, either accessed by reflection or using Python's |
|
I think the use case for providing metrics for direct implementations of |
…y-js into metrics-reader-metrics
Which problem is this PR solving?
I am helping to implement SDK internal metrics
https://opentelemetry.io/docs/specs/semconv/otel/sdk-metrics/
This implements metric reader metrics.
Implementation in Java - https://github.com/open-telemetry/opentelemetry-java/pull/8038/changes
/cc @trentm
Short description of the changes
Implement metrics for metric collections per semconv
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: